Skip to content

add interval#24793

Open
daviszhen wants to merge 15 commits into
matrixorigin:mainfrom
daviszhen:0601-add-interval
Open

add interval#24793
daviszhen wants to merge 15 commits into
matrixorigin:mainfrom
daviszhen:0601-add-interval

Conversation

@daviszhen

@daviszhen daviszhen commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #24486

What this PR does / why we need it:

增加interval函数

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@daviszhen

daviszhen commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

I found two blocking issues.

  1. The binder still steals some legal INTERVAL() function calls away from the new builtin path. In this patch base_binder.go rewrites interval(...) to T_interval whenever isIntervalExprSyntax(args) sees exactly two arguments and the second one is a string literal matching an interval unit. But the builtin itself is registered to accept string arguments, so calls like interval(5, 'day') never reach the new numeric INTERVAL() implementation even though they are valid under the registered signature. This keeps the old interval-expression path for a subset of function-style calls instead of fully disambiguating the new builtin.
  2. The string conversion semantics are not MySQL-compatible yet. makeIntervalParam() uses strconv.ParseFloat, so inputs such as '', ' ', '1abc', 'abc', and '10.9abc' raise hard errors. The new BVT file even documents the intended behavior as “return values with warnings, not fail”, but the checked-in .result records errors instead. That means the implementation and coverage are both locking in behavior that diverges from MySQL for the new function.

1,已经解决.
2, '', ' ', '1abc', 'abc', and '10.9abc' 之类的, mo一直是不兼容mysql的. 相关的bvt case 已经删除了.

@aunjgr aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MySQL INTERVAL(N, n1, n2, ...) comparison function — returns index of last value less than N, or 0 if N < n1, or -1 if N is NULL.

Binder disambiguation (base_binder.go): The old code in BindFuncExprImplByPlanExpr blindly rewrote ALL interval calls to ListExpr. The fix moves the rewrite to bindFuncExprImplByAstExpr where the AST shape is checked: only INTERVAL 5 DAY (2 args with *tree.TimeUnitExpr second arg) becomes ListExpr. INTERVAL(n, n1, n2) passes through to normal function binding.

Implementation (func_builtin.go): intervalParam wraps each arg vector with float/decimal accessors. Binary-search-like linear scan finds the first index where comparator < N. Supports int, uint, float, decimal, and string input types. Decimal comparison path preserves scale. Test matrix covers 20+ cases including the MySQL manual example.

Grammar: INTERVAL '(' bit_expr ',' expression_list ')' in mysql_sql.y — parser now recognizes the function form.

LGTM.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我这轮没有再挖到新的硬 correctness blocker,但按高标准看,当前 coverage 还差一条最关键的 parser/binder 回归用例。

这次 PR 的风险不只是 INTERVAL(...) 这个 builtin 本身能不能算对,更关键的是:加了它以后,现有的 INTERVAL 1 DAY / time-window rewrite 这些老语法有没有被误伤

现在我看到的是:

  • func_interval.sqlINTERVAL(...) 本身测得比较全;
  • binder/unit 侧也补了 disambiguation test;
  • query_builder.go 里把 time-window rewrite 改成了 NewTimeUnitExpr(...)

但还缺一条真正走到 SQL 执行层的混合回归,比如在同一条查询里同时出现:

  • INTERVAL(value, ...)
  • ts + INTERVAL 1 DAY 或 time-window 用法

这样才能真正锁住 parse → bind → rewrite → execute 这一整条链路没有被新 builtin 打歪。

所以我这边建议至少补一个很小的 SQL/BVT:把 INTERVAL(...)INTERVAL 1 DAY / time-window 语法放在同一条查询或同一个 case 里,确认两条语义都还对。代码本身这版基本可以,但这个产品级回归点我建议补上再过。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement kind/feature size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants